review: make the live eval a measurement tool (repeat aggregation, powered runs, drift watch)#252
Conversation
…live cases Phase 1 of the live A/B eval plan (#232). The corpus gains an opt-in live block carrying what a real model run needs and the deterministic replay does not: - prContext (PR title/description/author/base; the description is untrusted author text, so an adversarial case can carry its payload there or in the diff), - a post-change file tree on disk next to the case, via a new <id>/case.json + <id>/tree/ layout that coexists with flat <id>.json (a directory containing case.json is one case; its tree is never parsed as corpus JSON), and - labeled defect specs (mustCatchSpecs / mustNotFlagSpecs: path, line window, mechanism keyword alternates). Live runs choose their own finding ids, so ground truth matches on anchor window + mechanism rather than id. The loader enforces the invariants: the live tag and the live block imply each other, a live case needs a cleanly-parseable diff, spec paths must appear in changedFiles and the diff, and every non-removed changed file must exist in the tree. loadLiveCorpus() returns the subset. The live half lives in corpus/live.ts; loader.ts re-exports it as the single public surface. Ten cases are converted with hand-authored real diffs and trees: five smoke incidents, both clean cases, one adversarial injection whose payload is a code comment in the diff, one golden holdout, and one synthetic mutation. Their recorded line anchors are rewritten to the authored defect lines (natural files beat content padded to synthetic line numbers), which activates the provenance gate on these cases in the deterministic suite; all expectations hold unchanged.
…action and case staging Phases 2a/2b of the live A/B eval plan (#232), stacked on the Phase 1 corpus format (#233). agent-extract.ts turns review.md's '## agent:' sections into data (name, description, pinned model, prompt body). It takes the markdown as a string with no fs access, because the baseline arm of an A/B reads the merge-base review.md via git show. Parsing is strict; a malformed or model-less section throws listing every problem, since a silently dropped agent would skew an eval arm without failing it. An integration test extracts the real review.md (21 agents) and pins the staging-root reference so a future rename fails a test instead of silently staging nothing. live-stage.ts materializes the production staging layout for one live-enabled corpus case: pr-context.json (review.md Step 1's shape, synthetic identity fields), full.diff / pr.diff / full-stripped.diff (all the case diff; corpus diffs carry no generated files and no pattern-triage pass runs), files.json + review-files.json with the hasPatch cross-check derived from the diff parse, provenance.json, routing.json from the deterministic router, an out/ directory, and the post-change checkout copied from the case tree. rewriteAgentPrompt swaps the production staging root for the staged context dir. Everything sits behind an injected-fs seam and is memfs-tested. Model dispatch (phase 2c) is deliberately absent; it needs the Agent SDK dependency decision and arrives separately.
… runner (phase 2c)
live-producer.ts runs the live roster over one staged case behind an
injected LiveAgentRunner seam (the judge.ts pattern), so its logic is
stub-tested with zero model calls. Roster: the default finders
(correctness-reviewer, skill-auditor) plus the router's lensesToSpawn;
no pattern-triage or thread-reconciler in eval. It maps all three
sub-agent output contracts into the shapes the deterministic runner
consumes: label-shape findings (correctness lens, or conventions for
the skill-auditor so labelForFinding reproduces the best-practice
variants; confidence defaulted to 0.7 per the production claims rule),
structured-schema lens findings validated as-is, and the validator's
{claims: [...]} verdicts into CaseVerification[]. It stages
claims.json for the validator, resolves {{#runtime-import}} directives
against the case tree (a case opts into a skills index by carrying the
file), retries once on malformed output with the rejection fed back,
keeps partial results when an agent fails twice, prefixes colliding
finding ids, and reports per-agent cost/turns/wall-clock.
live-runner.ts is the one module that touches a real model runtime:
an Agent SDK query() per dispatch with Read/Grep/Glob only, cwd pinned
to the staged checkout, the agent's pinned model, and hard turn and
wall-clock caps; plus the CLI smoke entry
(tsx live-runner.ts --case <id>, requires ANTHROPIC_API_KEY). The
investigation-cap CLI the prompts reference is not staged; its own
denied-budget fallback applies. Adds @anthropic-ai/claude-agent-sdk
as a dev dependency.
live-match.ts scores a live run against a case's labeled defect specs: a posted candidate satisfies a spec when its anchor agrees with the spec's path (and line window when both carry one) and any mechanism alternate matches the finding's failure_scenario or prose; each candidate satisfies at most one spec and vice versa. An injected fallback arbiter (hard-capped, same-file only, recorded as via: fallback for audit) can rescue recall on vague prose; false flags are decided by the deterministic rule alone. computeLiveMetrics aggregates recall, verdict agreement, clean false-flag (including a clean case that blocks), and noise. live-ab.ts is the arm orchestrator and CLI: baseline review.md from git show <merge-base>, candidate from the working tree, both arms over the same live corpus with everything else (corpus, lib, runner, metrics, judge) from the candidate, per the plan's settled decision to isolate the model seam. Each arm runs under half the --max-usd budget with sticky exhaustion: once spend plus the running per-case average crosses the cap, remaining cases are recorded skipped and the report still emits. Spec-level regressions are diffed only over cases both arms scored. Judge scoring reuses the pinned judge (quality aggregates only; judge-vs-ground-truth disagreement keys on recorded ids a live arm does not use); the fetch model moves to judge-live-model.ts and live-judge.ts now imports it. runner.ts gains an optional RunOptions.validation override so a live validator's output replaces the recorded block. Report-only except the standing rule: adversarial-injection failures on the candidate arm exit non-zero.
Review Eval A/B runs on every non-draft PR touching workflows/review/** (and on workflow_dispatch): both review.md arms over the live corpus via live-ab.ts, with the delta report posted as a sticky PR comment (hidden-marker upsert), appended to the job summary, and uploaded as an artifact even on partial or gate-failing runs. Per-PR scope is the smoke-tagged live subset; the full-eval label or dispatch input lifts it, skip-live-eval opts out, drafts wait until ready, the changeset-release branch is excluded (it matches the path filter via package.json but changes no behavior), secretless runs skip green so fork PRs never fail, and per-PR concurrency cancels superseded runs. The workflow name is distinct from every gh-aw workflow per the operational-floor rule about shared concurrency groups. live-ab.ts gains --smoke-only and writes out/live-ab-report.md alongside the JSON for the comment step.
… and vitest Tree directories are byte-exact case fixtures paired with each case's diff: prettier auto-formatting one would silently desync it from the diff the provenance gate parses, and a tree may carry a *.test.ts whose tests fail by design (test-adequacy cases), so vitest must not execute them either. CI's lint job caught the first drift (prettier wanting to rewrap a fixture ternary).
…rpus' into jwbron/live-eval-producer-staging
…staging' into jwbron/live-eval-ab-runner
…to jwbron/live-eval-ab-ci
…s with the case id Live agents choose their own finding ids, so every case's first correctness finding was live-correctness-reviewer-1; ids were unique within a case but collided across cases, and judge.ts's score join requires arm-global uniqueness. Caught by the first real A/B run (both arms completed, then judge aggregation threw). Ids are now <caseId>:<id> from the moment of parsing, so claims.json, the validator round-trip, the matcher, and the judge all see the same namespaced id.
…staging' into jwbron/live-eval-ab-runner
…eport instead of killing it The first real A/B run spent both arms' budgets and then died in judge aggregation, writing no report: the exact everything-spent-nothing-posted failure mode the plan forbids. Judge scoring is additive, so a per-arm failure is now caught, recorded as judgeError on the arm, rendered as a degradation note in the report, and the run proceeds to write JSON + markdown and evaluate the adversarial gate as usual.
…to jwbron/live-eval-ab-ci
…A/B phase 5) Packages the seeded-defect live-trial pattern (Khan/webapp#40678) as a Claude Code skill so a trial costs an operator an afternoon instead of a week of hand choreography. The skill collects required inputs (seeded branch, human-authored defect table, arms, budget approval) and refuses to improvise ground truth; sets up one isolated PR per arm with per-arm trigger recipes and the distinct-workflow-name rule (same-named gh-aw workflows share a per-PR concurrency group and cancel each other); collects reviews, artifacts, and costs per run with the known gh-aw artifact-bug tolerance; drives optional lifecycle pushes; scores defect by defect with the deterministic rule mirroring eval/live-match.ts plus audited manual judgment; exports live-enabled corpus case skeletons with a sanitization gate for private-repo content landing in this public repo; and cleans up trial PRs, branches, and temporary workflows. Trials remain the architecture-bet instrument; per-change evals belong to the corpus A/B. .gitignore narrows from .claude to .claude/* with a !.claude/skills/ carve-out: local agent state (settings, worktrees) stays untracked, project skills are shared tooling and are committed.
…orpus' into tmp-refresh
…roducer-staging' into tmp-refresh
…b-runner' into tmp-refresh
…b-ci' into tmp-refresh
…ity, code-rendered from the reconciler keep-list
… instead of dropping them
…oad (allowed-paths must match staging-relative paths)
…untability' into jwbron/review-out-of-lane
…into one staged disciplines file
…ive case The first live acceptance runs missed incident-sql-missing-index:dm-missing-index-1 in all four arms: the recorded finding is a data-migrations LENS finding, but live cases carried no routerConfig lens rules, so the live roster never spawned the specialist that catches it. Every live case whose recorded finding belongs to a specialist lens now routes that lens on the finding's file (seven cases across five lenses), mirroring how a consumer ROUTING file would route the same paths in production.
…rpus' into jwbron/live-eval-producer-staging
…staging' into jwbron/live-eval-ab-runner
… the A/B report The acceptance runs surfaced a claim-validator failure that the report could only name, not explain (perCase carried agent names only, and the PerAgentReport.failed detail never reached the markdown). perCase failedAgents entries are now '<agent>: <reason>', so the next failure is diagnosable from the sticky comment alone.
…rom the main README
Main vs stack tip: the cumulative measurement, and how to read itPooled from the three 2026-07-10 full-corpus dispatches (29065968954 / 29065976210 / 29065977160; baseline
Verdict: recall-neutral, which is the designed outcome. Batch two is process and output-quality work (accountability, rendering, re-review machinery); the A/B's job for that class of change is to prove nothing silently regressed across a heavy prompt rework, and it did: the candidate arm has zero true misses. All movement sits inside the measured noise floor (recall wobbles 54-86% on byte-identical arms; see the report footer this PR adds). What the run actually taught us:
Reading future reports: the next drift runs will show recall and noise 'improving' because this PR fixed the ruler (altLocations) and de-noises matching (arbiter), not because the reviewer changed. Compare like with like via the review.md shas and the noise-floor provenance in each report. |
…-floor statistics, rolling repeat budgets Findings from an adversarial pass over the instrument's own claims: Reports now stamp their ruler (matcher configuration + a content hash of the loaded corpus); the aggregate prints it, warns when a pool mixes rulers, and the drift series stays interpretable across instrument upgrades like the arbiter default or corpus growth, which move every rate without the reviewer changing. Noise-floor bands gain SD (min/max are extreme-value statistics that only widen as samples accumulate; mean +/- sd is the stable band) and a loud case-asymmetry warning: the 2026-07-10 measurement had budget skips, so its published v1 bands fold case-mix variance in on top of wobble; the footer constants and provenance string now say so. Repeat budgets roll: each arm-run's slice is the remaining budget over the remaining arm-runs, so cheap early arms donate headroom forward instead of stranding it (fixed slices caused the 38/36-of-42 skip asymmetry that contaminated the noise-floor run). README gains a statistical-honesty section: clustered-interval optimism, the repeats-gate relaxation, and the arbiter's uncalibrated refuse bias.
|
One more commit (2369b86) from an adversarial pass over the instrument's own claims:
Known-open items live in the tuning memo rather than this stack: arbiter false-positive calibration, the gate-policy decision, a post-merge validation dispatch of the drift PR flow, and a clean re-measurement of the noise floor at the $85 budget. |
…ereview-accountability
| * prose. The weekly drift run re-measures these bands (with the arbiter | ||
| * active); update the constants when they move materially. | ||
| */ | ||
| export const MEASURED_NOISE_FLOOR = { |
There was a problem hiding this comment.
thought (non-blocking): MEASURED_NOISE_FLOOR's provenance says "pre-arbiter", but this PR makes the Haiku match arbiter the default matcher, so every single-run report footer prices arbiter-era deltas against pre-arbiter wobble bands until someone hand-edits the constant. Consider warning when the renderer's provenance.matcher differs from the noise floor's ruler, or sourcing the bands from the latest committed drift aggregate.
| # the memo's answer: the FULL live corpus, 3 repeats per arm, both arms | ||
| # pinned to main's review.md (`--force-arms` past the identity | ||
| # short-circuit), aggregated into per-case pass rates with binomial | ||
| # intervals. Week-over-week comparison happens across artifacts; within one |
There was a problem hiding this comment.
suggestion (non-blocking): The drift watch emits weekly snapshots but nothing computes the week-over-week comparison — the header notes it "happens across artifacts" (i.e. by a human), so a slow recall regression on main would sail under the within-run adversarial gate. aggregate.ts already accepts run ids, so feeding the current run plus the last N committed drift JSONs through it would render prior-week bands next to this week's and make the watch self-comparing.
| STAMP="$(date -u +%Y-%m-%d)" | ||
| DIR=".github/review-eval/drift" | ||
| BRANCH="drift-report/${STAMP}-${{ github.run_id }}" | ||
| mkdir -p "$DIR" |
There was a problem hiding this comment.
note (non-blocking): This workflow grants contents: write + pull-requests: write on a schedule trigger and opens a PR on every run (if: always()). The interpolations are all repo-controlled, so there is no injection surface — flagging only for visibility that the write-scope and the per-run branch/PR accumulation are intended.
| // Fallback: only candidates sharing a spec file, in posted order. | ||
| for (const candidate of posted) { | ||
| if (claimed.has(candidate.id) || candidate.path !== spec.path) { | ||
| if (claimed.has(candidate.id) || !onSpecFile(candidate, spec)) { |
There was a problem hiding this comment.
suggestion (non-blocking): onSpecFile's new altLocations awareness (this fallback gate and the drop classifier at line 254) is only exercised via anchorAgrees; every matchCase test uses a candidate on the primary spec path. A spec whose real finding lands on the alt file only would be scored a true miss rather than a fallback/found-but-dropped catch, and that path is unverified. Consider a matchCase test with an alt-file candidate.
…array, not the scalar count
The per-repeat checkpoint wrote {repeats: <n>} where the recovery path
in aggregate.ts expects {repeats: [reports]}; a crash mid-run therefore
forfeited every completed repeat, which is exactly what the checkpoint
exists to prevent. Write the reports array, matching the final artifact
shape, and lock the checkpoint-to-extractSamples contract with a test.
This comment has been minimized.
This comment has been minimized.
| let armRunsDone = 0; | ||
| let spentUsd = 0; | ||
| const nextArmBudget = (): number => | ||
| Math.max(0, maxUsd - spentUsd) / (totalArmRuns - armRunsDone); |
There was a problem hiding this comment.
note (non-blocking): Judge and arbiter spend are excluded from --max-usd and scale with --repeats; gate-retry spend is also untracked. The rolling budget only accounts for arm-producer cost (spentUsd += report.usd), and nextArmBudget divides only the remaining maxUsd - spentUsd. Judge cost (liveJudgeModel, one paid call per finding per arm) and the Haiku arbiter cost (haikuMatchArbiter) never flow into spentUsd, so --repeats n multiplies that untracked spend across 2*n arm-runs. Gate-retry cost is likewise untracked (though it runs only on the single-run path). Consider folding these into spentUsd, or documenting that --max-usd bounds arm-producer spend only.
…; pin github-script to v8 Two review nitpicks from the stack: live-match.ts imported two types from ./corpus/loader in separate statements where the rest of the eval dir merges them, and the sticky-comment step pinned github-script v7 while node-ci.yml and release.yml pin v8 (ed597411). Align both.
Review Guidancegithub-actions (7 files)
Common patterns4 files: altLocations support — LiveDefectSpec gains an altLocations field (type + parser in live.ts, matcher in live-match.ts), validated in loader.test.ts and used in the smoke case fixture. 2 files: Report types and renderers extracted from live-ab.ts into the new live-ab-report.ts leaf module; live-ab.ts re-exports them to preserve the import surface. 2 files: One-line re-export propagation — LiveSpecLocation added to the loader barrel and the altLocations field added to the smoke case.json. Excluded from review (6 files)Not individually reviewed — generated, formatting-only, or fully explained by a common pattern above:
|
| - name: Post the sticky PR comment | ||
| if: always() && github.event_name == 'pull_request' | ||
| uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7 | ||
| uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8 |
There was a problem hiding this comment.
note (non-blocking): This v8 SHA matches the pin already used by node-ci.yml and release.yml, so the alignment is correct. Heads up that .github/workflows/review-rereview-sweep.yml:79 is now the last workflow still on the v7 SHA — a trivial follow-up would bring the repo to a single pinned version.
…ain' into jwbron/eval-measurement-tool # Conflicts: # .github/workflows/review-eval-ab.yml # workflows/review/eval/corpus/live.ts # workflows/review/eval/corpus/loader.test.ts # workflows/review/eval/corpus/loader.ts # workflows/review/eval/corpus/smoke/incident-sql-missing-index/case.json # workflows/review/eval/live-ab.test.ts # workflows/review/eval/live-ab.ts # workflows/review/eval/live-match.test.ts # workflows/review/eval/live-match.ts # workflows/review/lib/disciplines.test.ts # workflows/review/lib/rereview-mode.ts # workflows/review/review.md
Lands the tuning memo's rev-2 measurement items (memo: "Dialing in the evals", rev 2). The per-PR A/B stays a tripwire; this adds the machinery that turns repeats into resolution.
What's here
Repeat-aggregation report (
eval/aggregate.ts, memo item 3). Deterministic core + thin CLI: pools Nlive-ab-report.jsonartifacts (local paths orghrun ids) into per-case pass rates per arm with 95% Wilson intervals, pooled recall/verdict/noise rows, and true-miss vs found-but-dropped classification per gate bucket. Warns when a pool mixes review.md shas. Validated against the three 07-10 cumulative dispatches (29065968954 / 29065976210 / 29065977160): reproduces the memo's hand-computed numbers exactly (baseline 21/24 specs, 28/30 verdicts; candidate 22/24, 28/30).--repeats <n>inlive-ab.ts(memo sizing section). One dispatch runs the selected cases n times per arm (budget split per arm-run, fresh staging per repeat) and reports through the aggregation core instead of single-run percentages; this is the ~$29 targeted-repeats powered run (--cases <two anchor-fragile cases> --repeats 10). The adversarial gate is decided by strict majority across repeats; the single-run best-of-three retry is skipped since the repeat structure buys the same flake evidence. The A/B workflow exposescases/repeats/force_armsdispatch inputs.Scheduled drift watch (
review-eval-drift.yml). Weekly cron: full live corpus x3 repeats, both arms pinned to main's review.md (--force-armspast the identity short-circuit), aggregate published to job summary + artifact. Identical arms mean 6 arm-samples of the same prompt per run, so every drift run also refreshes the noise-floor bands, which the aggregate renders as data (memo item 6).incident-sql-missing-index diagnosed and fixed (memo item 2). Read every recorded transcript in the wave + cumulative artifacts (28 arm-runs): the reviewer found the missing index in 28/28. The 8/16 catch rate was the spec accepting only the migration-file anchor while the reviewer anchored the same blocking finding at the hot query in
src/models/order.ts(11 "misses") or had it provenance-dropped on an out-of-file mis-anchor (2, the adversarial-injection-approve pathology). Neither mechanism alternates nor lens routing were at fault; the case measured anchor-site preference. Specs gainaltLocations(validated like the primary location; matcher, fallback filter, and drop classification all honor them) and the case accepts the query-site window. Replayed against all 28 recorded runs: 26 caught, 2 found-but-dropped, 0 true misses; the residual failure class is exactly the anchor-snap production defect, now correctly bucketed.Notes
pnpm test --run(879 tests) andpnpm typecheckpass; eslint can't run in this worktree (dot-dir ignore), prettier applied.